[SYCL] Add validation + exception handling to SYCL_PARALLEL_FOR_RANGE_ROUNDING_PARAMS #19381
Conversation
AlexeySachkov
left a comment
AlexeySachkov
left a comment
There was a problem hiding this comment.
We should have a test added for the change
|
@intel/llvm-reviewers-runtime PR ready for review, thanks in advance! |
| if (Pos != std::string::npos) { | ||
| MF = std::stoi(Params.substr(0, Pos)); | ||
| if (Pos != std::string::npos && GuardedStoi(MF, Params.substr(0, Pos)) && | ||
| MF > 0) { |
There was a problem hiding this comment.
should this be MF >= 0 ( GTE instead of GT) ? 0 is a valid return from GuardedStoi, no? Or maybe 0 should not be valid in GuardedStoi.
There was a problem hiding this comment.
The way we currently have range rounding, MF=0 results in a division by zero error: This PR helps prevent the crash that would happen as a result.
I don't think it makes sense anyway, as according to https://github.com/intel/llvm/blob/970e2ef386e6287202cbaf69ff3871889bed5959/sycl/doc/EnvironmentVariables.md, the minimum factor is the "range that the rounded range should be a multiple of (Default 16)": Rounding parallel_for ranges to a multiple of 0 should probably not be possible. Thus, I have a test case to explicitly check that MF=0 should not be accepted.
|
@cperkinsintel Mind if I ask you to take a look again? Thanks! |
|
@intel/llvm-gatekeepers PR is ready for merge, thanks! |
|
Can you add a PR description? |
Done 👍 |
|
Thx! |
4 participants
Bad / nonsensical values in
SYCL_PARALLEL_FOR_RANGE_ROUNDING_PARAMSresult in division by 0 crashes. This PR: